Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Tapioca LSP #2001

Closed
wants to merge 32 commits into from
Closed

WIP: Tapioca LSP #2001

wants to merge 32 commits into from

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Aug 26, 2024

sig { params(changes: T::Array[{ uri: String, type: Integer }]).void }
def workspace_did_change_watched_files(changes)
# TODO: avoid direct access
files_to_entries = @index.instance_variable_get("@files_to_entries")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vinistock how do you feel about the Tapioca DSL directly accessing this?

(cc @KaanOzkan)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like for all of this logic to live inside the index object, it can give us the entries from a given file. Dealing with files_to_entries here is too internal and we'd need an attr_reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paired with Vini on this a bit earlier. We now have a method on @index that can be called.

@index.entries_for(path, RubyIndexer::Entry::Namespace)

entries.map(&:name)
end.flatten.compact

# TODO: confirm with kaan if we expect a response here? (i.e. should it be a notificatio or a request)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KaanOzkan do you think see situation where the Tapioca LSP will need to provide a response? Or will it be 'fire-and-forget'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how responses are used by the client. End result of what we need is logs in the output tab and potentially an error notification popup (Not sure).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it flexible and allow for the possibility of some error that we want to display. I will change it to be a request.

@KaanOzkan
Copy link
Contributor

Closing this since we aren't using it for discussion anymore and we'll have more shippable PRs coming soon.

@KaanOzkan KaanOzkan closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants